AWSOIDC: Collect required vpcs and its subnets for use in web UI by kimlisa · Pull Request #35930 · gravitational/teleport

kimlisa · 2023-12-20T10:15:52Z

Web UI requires to get a map of missing vpc and its subnets so that auto-deploy step can deploy services for the missing vpcs. If there are no missing vpcs, then user can skip the auto-deploy screen.

Here is overview:

collect all rds instances and clusters into one, and create a map of vpc and its subnets
collect all database services with the fargate/ecs label
- iterate through these services and look for exact label matches for:
  - account-id
  - region
  - vpc id

github-actions · 2023-12-20T19:37:24Z

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

marcoandredinis

I still want to pass again, specially on the logic where we remove the VPCs
I think it's ok, but I need fresh eyes for this 😅

marcoandredinis · 2023-12-20T19:50:19Z

We discussed this over slack, but this might not return the expected DB Services while the deployed version is <14.2.4

are you suggesting that i remove it? (or just a FYI during testing?)

FYI during testing.

I will try to create a dev build and maybe we can use that during our tests
It will be based on the current v14 branch which includes the new label

You can hard-code the following version if you want to check for the label

teleportVersionTag := "14.2.5-dev.marco.2"

Example:
$ tctl get db_services

kind: db_service metadata: expires: "2023-12-21T15:44:50.327704669Z" id: 1703172890371535865 labels: teleport.dev/awsoidc-agent: "true" name: 05c13b7c-ed93-42de-9f43-753c1afc08f2 revision: fba61218-27ea-45c9-ad81-3ff2384a7c1c spec: resources: - aws: {} labels: account-id: "278576220453" region: us-east-1 vpc-id: vpc-092abc version: v1

worked great 👍

lisakim ~/gravitational/teleport/e/build [master] $ ./tctl get db_services kind: db_service metadata: expires: "2023-12-22T04:55:30Z" id: 1703218345695400557 labels: teleport.dev/awsoidc-agent: "true" name: 9cdde9be-9bd5-4c12-b203-363df6059e5c spec: resources: - aws: {} labels: account-id: "278576220453" region: us-east-1 vpc-id: vpc-02149278b986b6f83 version: v1 --- kind: db_service metadata: expires: "2023-12-22T04:55:14Z" id: 1703218339966369543 labels: teleport.dev/awsoidc-agent: "true" name: fb7abf52-a70b-4253-849a-da7ebe1cb92d spec: resources: - aws: {} labels: account-id: "278576220453" region: us-east-1 vpc-id: vpc-092c26a0e0e802e92 version: v1

marcoandredinis

Just left some comments

marcoandredinis · 2023-12-21T08:52:19Z

FYI during testing.

I will try to create a dev build and maybe we can use that during our tests
It will be based on the current v14 branch which includes the new label

marcoandredinis · 2023-12-21T11:19:57Z

I would do some assumptions here: we are only interested in the DatabaseServices deployed by us, which have a known configuration.

We should only have a single ResourceMatcher with non-empty Label set, and each Label must have a single LabelValue.
This should allow us to simplify the multiple for loops we have right now.

Suggested change

// Start looking for db service matches.

wantLabels := map[string][]string{

types.DiscoveryLabelAccountID: {},

types.DiscoveryLabelRegion: {},

types.DiscoveryLabelVPCID: {},

}

for _, svc := range fetchedDbSvcs {

// Create a lookup table of labels for easier matching.

labelLookup := map[string][]string{}

for _, matcher := range svc.GetResourceMatchers() {

for key, newVals := range *matcher.Labels {

if existingVals, ok := labelLookup[key]; ok {

labelLookup[key] = append(existingVals, newVals...)

continue

}

labelLookup[key] = newVals

}

}

// Do an exact match, b/c other labels may not match.

if len(labelLookup) != len(wantLabels) {

continue

}

// Match labels contains the keys we are looking for.

matchedLabelKeys := true

for key := range wantLabels {

vals, found := labelLookup[key]

if !found {

matchedLabelKeys = false

break

}

wantLabels[key] = vals

}

if matchedLabelKeys &&

slices.Contains(wantLabels[types.DiscoveryLabelAccountID], req.AccountID) &&

slices.Contains(wantLabels[types.DiscoveryLabelRegion], req.Region) {

// Delete found vpcs

vpcs := wantLabels[types.DiscoveryLabelVPCID]

for _, vpc := range vpcs {

delete(vpcLookup, vpc)

}

}

}

for _, svc := range fetchedDbSvcs {

if len(svc.GetResourceMatchers()) != 1 || svc.GetResourceMatchers()[0].Labels == nil {

continue

}

labelMatcher := *svc.GetResourceMatchers()[0].Labels

if len(labelMatcher) != 3 {

continue

}

if slices.Compare(labelMatcher[types.DiscoveryLabelAccountID], []string{req.AccountID}) != 0 {

continue

}

if slices.Compare(labelMatcher[types.DiscoveryLabelRegion], []string{req.Region}) != 0 {

continue

}

if len(labelMatcher[types.DiscoveryLabelVPCID]) != 1 {

continue

}

delete(vpcLookup, labelMatcher[types.DiscoveryLabelVPCID][0])

}

Tests are failing with this change, but that's because we have multiple ResourceMatchers on the DatabaseServices configuration we created

marcoandredinis · 2023-12-21T11:24:56Z

I think we should add more information about what this endpoint does and where it is used.

kimlisa · 2023-12-22T05:05:06Z

friendly ping @strideynet @EdwardDowling

marcoandredinis · 2023-12-22T08:28:41Z

+func stringPointer(s string) *string {
+	return &s
+}


💅
I think we should use aws.String() instead.
I used this method in the past, but given that it was only being used for AWS Types, I converted most of them to just use the auxiliary method from the AWS package

strideynet · 2023-12-22T11:36:48Z

 	}
 }
+
+func TestAWSOIDCRequiredVPCSHelper(t *testing.T) {


Could these tests be t.Parallel() ?

public-teleport-github-review-bot · 2023-12-22T16:49:55Z

@kimlisa See the table below for backport results.

Branch	Result
branch/v14	Failed

) * Discover: collect all rds vpc ids and its subnets * Address Cr * Add api endpoint to web config * Address Cr * Fix web api route * Address CR

#35930) (#36016) * AWSOIDC: Collect required vpcs and its subnets for use in web UI (#35930) * Discover: collect all rds vpc ids and its subnets * Address Cr * Add api endpoint to web config * Address Cr * Fix web api route * Address CR * Fix lint

kimlisa force-pushed the lisa/collect-required-vpcs branch from 6802164 to 0a8d2c4 Compare December 20, 2023 19:33

kimlisa marked this pull request as ready for review December 20, 2023 19:37

kimlisa requested a review from marcoandredinis December 20, 2023 19:37

github-actions Bot added the size/md label Dec 20, 2023

github-actions Bot requested review from EdwardDowling and strideynet December 20, 2023 19:37

kimlisa added no-changelog Indicates that a PR does not require a changelog entry backport/branch/v14 labels Dec 20, 2023

marcoandredinis reviewed Dec 20, 2023

View reviewed changes

kimlisa force-pushed the lisa/collect-required-vpcs branch from eb56d8b to 0885e70 Compare December 20, 2023 20:51

kimlisa changed the title ~~AutoDiscover: Collect required vpcs and its subnets for use in web UI~~ AWSOIDC: Collect required vpcs and its subnets for use in web UI Dec 21, 2023

marcoandredinis reviewed Dec 21, 2023

View reviewed changes

kimlisa added 3 commits December 21, 2023 14:38

Discover: collect all rds vpc ids and its subnets

bcff725

Address Cr

3a51943

Add api endpoint to web config

ef2ddbb

kimlisa force-pushed the lisa/collect-required-vpcs branch from 0885e70 to 43e099f Compare December 21, 2023 22:46

Address Cr

aacc1af

kimlisa force-pushed the lisa/collect-required-vpcs branch from 43e099f to aacc1af Compare December 21, 2023 23:22

kimlisa added 2 commits December 21, 2023 20:44

Fix web api route

b6a0b34

Merge branch 'master' into lisa/collect-required-vpcs

6593b98

kimlisa mentioned this pull request Dec 22, 2023

Web: Add auto enrolling capabilities to RDS discover flow #35646

Merged

marcoandredinis approved these changes Dec 22, 2023

View reviewed changes

strideynet approved these changes Dec 22, 2023

View reviewed changes

public-teleport-github-review-bot Bot removed the request for review from EdwardDowling December 22, 2023 11:37

kimlisa added 2 commits December 22, 2023 08:10

Address CR

13c0472

Merge branch 'master' into lisa/collect-required-vpcs

622c1d5

kimlisa enabled auto-merge December 22, 2023 16:11

kimlisa added this pull request to the merge queue Dec 22, 2023

Merged via the queue into master with commit 386c652 Dec 22, 2023

kimlisa deleted the lisa/collect-required-vpcs branch December 22, 2023 16:48

kimlisa mentioned this pull request Dec 22, 2023

[v14] AWSOIDC: Collect required vpcs and its subnets for use in web UI (#35930) #36016

Merged

-	// Start looking for db service matches.
-	wantLabels := map[string][]string{
-		types.DiscoveryLabelAccountID: {},
-		types.DiscoveryLabelRegion:    {},
-		types.DiscoveryLabelVPCID:     {},
-	}
-	for _, svc := range fetchedDbSvcs {
-		// Create a lookup table of labels for easier matching.
-		labelLookup := map[string][]string{}
-		for _, matcher := range svc.GetResourceMatchers() {
-			for key, newVals := range *matcher.Labels {
-				if existingVals, ok := labelLookup[key]; ok {
-					labelLookup[key] = append(existingVals, newVals...)
-					continue
-				}
-				labelLookup[key] = newVals
-			}
-		}
-		// Do an exact match, b/c other labels may not match.
-		if len(labelLookup) != len(wantLabels) {
-			continue
-		}
-		// Match labels contains the keys we are looking for.
-		matchedLabelKeys := true
-		for key := range wantLabels {
-			vals, found := labelLookup[key]
-			if !found {
-				matchedLabelKeys = false
-				break
-			}
-			wantLabels[key] = vals
-		}
-		if matchedLabelKeys &&
-			slices.Contains(wantLabels[types.DiscoveryLabelAccountID], req.AccountID) &&
-			slices.Contains(wantLabels[types.DiscoveryLabelRegion], req.Region) {
-			// Delete found vpcs
-			vpcs := wantLabels[types.DiscoveryLabelVPCID]
-			for _, vpc := range vpcs {
-				delete(vpcLookup, vpc)
-			}
-		}
-	}
+	for _, svc := range fetchedDbSvcs {
+		if len(svc.GetResourceMatchers()) != 1 || svc.GetResourceMatchers()[0].Labels == nil {
+			continue
+		}
+		labelMatcher := *svc.GetResourceMatchers()[0].Labels
+		if len(labelMatcher) != 3 {
+			continue
+		}
+		if slices.Compare(labelMatcher[types.DiscoveryLabelAccountID], []string{req.AccountID}) != 0 {
+			continue
+		}
+		if slices.Compare(labelMatcher[types.DiscoveryLabelRegion], []string{req.Region}) != 0 {
+			continue
+		}
+		if len(labelMatcher[types.DiscoveryLabelVPCID]) != 1 {
+			continue
+		}
+		delete(vpcLookup, labelMatcher[types.DiscoveryLabelVPCID][0])
+	}

Conversation

kimlisa commented Dec 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Dec 20, 2023

Uh oh!

marcoandredinis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

marcoandredinis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kimlisa commented Dec 22, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

public-teleport-github-review-bot Bot commented Dec 22, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kimlisa commented Dec 20, 2023 •

edited

Loading